Add optional commit SHA in config.#93
Conversation
jefchien
force-pushed
the
add-optional-commit-sha
branch
from
4ab55a4 to
35c0db2
Compare
jefchien
force-pushed
the
add-optional-commit-sha
branch
from
35c0db2 to
dd836ac
Compare
|
|
||
| async function getCommit(githubToken?: string): Promise<Commit> { | ||
| async function getCommit(githubToken?: string, commitSha?: string): Promise<Commit> { | ||
| if (commitSha && githubToken) { |
There was a problem hiding this comment.
Should this line be placed after the line 253-261 ? Mostly I can't see any reason to let this success faster though.
There was a problem hiding this comment.
Well, if you've provided the commit SHA, then your intention is for the action to use it. I don't see why it should even attempt to use the other options.
|
Hi @jefchien Thank you for your contribution! Could you please explain to me a bit more why would we want to provide the SHA as an input rather than just using the one provided via pull request or fall back to the current commit's sha? |
|
Hi @ktrz, So, in my use case, the workflow where the action is used can be triggered by a workflow dispatch where the SHA is provided as an input. This allows us to run the benchmarking after successful CI workflow runs or on selected commits. The issue I've found is that the |
|
Is running the That way you can make the branch point to the commit you want, and run the workflow from that branch, so that the commit is correct and you don't need to provide the It might be more tricky if your invocation of the |
|
Running the |
|
Hi @ktrz , any feedback though? |
|
I'll get back to this during the weekend as I have a pretty busy week |
ktrz
left a comment
ktrz
left a comment
There was a problem hiding this comment.
The PR looks good. Just need some merge conflicts resolved. And please also include a test specific for your use-case
| externalDataJsonPath: undefined, | ||
| maxItemsInChart: null, | ||
| failThreshold: 2.0, | ||
| commitSha: 'dummy sha', |
There was a problem hiding this comment.
I think we should add specific tests for the use-case where we set the explicit commitSha rather than just adding this param here.
4 participants
Description
Allows the user to pass in a commit SHA to use instead of relying on the payload or head commit. The main use-case of this is for workflow dispatch runs where the SHA is provided through an input.
This change should be valid based on https://docs.github.com/en/rest/reference/commits#get-a-commit.